Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryThis PR introduces three new read-only copilot tools (
Confidence Score: 4/5Safe to merge with the args nullability issue resolved — all other changes are additive registrations and a well-contained bug fix. The core logic (read-only guards, tool registration, schema sync, bug fix in terminal data) is sound and well-commented. One concern is the apps/sim/lib/copilot/tools/server/knowledge/search-knowledge-base.ts and apps/sim/lib/copilot/tools/server/table/query-user-table.ts — both type Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Copilot Model Request] --> B{Tool Route}
B -->|run_code| C[executeRunCode]
C --> D{outputs / outputTable in params?}
D -->|Yes| E[Return error: compute-only]
D -->|No| F[executeFunctionExecute sandbox execution]
B -->|search_knowledge_base| G[searchKnowledgeBaseServerTool]
G --> H{operation in READ_OPERATIONS?}
H -->|No| I[Return error: read-only]
H -->|Yes| J[knowledgeBaseServerTool.execute]
B -->|query_user_table| K[queryUserTableServerTool]
K --> L{operation in READ_OPERATIONS?}
L -->|No| M[Return error: read-only]
L -->|Yes| N{outputPath present?}
N -->|Yes| O[Return error: read-only]
N -->|No| P[userTableServerTool.execute]
B -->|search subagent| Q[Search Subagent replaces Research]
F & J & P & Q --> R[Tool Result]
R --> S[getToolCallTerminalData]
S --> T{failed AND output defined?}
T -->|Yes| U[Merge error into output for model context]
T -->|No| V[Return output or error as-is]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Copilot Model Request] --> B{Tool Route}
B -->|run_code| C[executeRunCode]
C --> D{outputs / outputTable in params?}
D -->|Yes| E[Return error: compute-only]
D -->|No| F[executeFunctionExecute sandbox execution]
B -->|search_knowledge_base| G[searchKnowledgeBaseServerTool]
G --> H{operation in READ_OPERATIONS?}
H -->|No| I[Return error: read-only]
H -->|Yes| J[knowledgeBaseServerTool.execute]
B -->|query_user_table| K[queryUserTableServerTool]
K --> L{operation in READ_OPERATIONS?}
L -->|No| M[Return error: read-only]
L -->|Yes| N{outputPath present?}
N -->|Yes| O[Return error: read-only]
N -->|No| P[userTableServerTool.execute]
B -->|search subagent| Q[Search Subagent replaces Research]
F & J & P & Q --> R[Tool Result]
R --> S[getToolCallTerminalData]
S --> T{failed AND output defined?}
T -->|Yes| U[Merge error into output for model context]
T -->|No| V[Return output or error as-is]
Reviews (1): Last reviewed commit: "fix(copilot): failed tool calls must sur..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (params && 'outputPath' in (params as Record<string, unknown>)) { | ||
| return { | ||
| success: false, | ||
| message: | ||
| 'query_user_table is read-only: outputPath (file export) is not available; digest the rows directly or route exports through the table agent', | ||
| } | ||
| } |
There was a problem hiding this comment.
outputPath check
The check at lines 35–40 already guards against outputPath nested inside params.args. The second check at lines 42–48 looks for outputPath at the top level of params, but outputPath is not a valid top-level field in QueryUserTableArgs — the catalog schema only defines operation and args at this level. If the Go executor or the server framework normalises the payload into the typed shape before reaching this handler, the top-level path can never be reached, making this check dead code. If the raw payload could arrive unnormalised, a note explaining that assumption would help clarify intent.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| type SearchKnowledgeBaseArgs = { | ||
| operation: string | ||
| args?: Record<string, any> | ||
| } |
There was a problem hiding this comment.
args typed optional but required in schema
SearchKnowledgeBaseArgs.args is typed as optional (args?: Record<string, any>), yet the generated catalog entry at SearchKnowledgeBase.parameters.required includes 'args'. The same pattern exists in QueryUserTableArgs. If the LLM omits args, the handler forwards params with args: undefined to knowledgeBaseServerTool.execute, where downstream property accesses like params.args.knowledgeBaseId would throw. Aligning the TypeScript type with the schema (args: Record<string, any> without ?) makes the contract explicit and prevents a runtime crash on malformed calls.
PR SummaryMedium Risk Overview OAuth2 authorize now supports optional Mothership chat fork copies uploads born at or before the fork point: storage quota gate, physical blob copies with new ids/keys, transcript and resource reference rewrites, ghost resource cleanup, failed-copy row deletion +
Function execute returns 422 when sandbox mounts/exports are requested without E2B; overwrite exports get byte-compare unchanged warnings, size/sha256 receipts, and a leading newline on E2B result markers. Streamed workflow remove events evict workflows from the active React Query cache. Smaller changes: credential reconnect link labels, Reviewed by Cursor Bugbot for commit 07af501. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| answersByIndex[index] = answers | ||
| hiddenUserByIndex[index + 1] = true | ||
| } | ||
| return { answersByIndex, hiddenUserByIndex } |
There was a problem hiding this comment.
Last tag wins pairing
Medium Severity
When one assistant turn contains more than one question segment, pairing uses only the last tag’s schema via parseLastQuestionTag, but every QuestionDisplay on that turn receives the same questionAnswers. Earlier cards can show another batch’s prompts/answers or blank recap lines.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fcfacf9. Configure here.
| setStep(next) | ||
| const prefill = customByStep[next] ?? '' | ||
| setFreeText(prefill) | ||
| setFreeTextEditing(prefill.trim().length > 0) |
There was a problem hiding this comment.
Stepper ignores committed custom text
Medium Severity
In multi-step question cards, goToStep calls commitCustom() but reads the prefilled “Something else” value from stale customByStep instead of the array commitCustom returns. After moving forward and back, earlier typed custom answers can disappear from the input even though they were just committed.
Reviewed by Cursor Bugbot for commit 8b6c2df. Configure here.
| if (!lines[i].startsWith(prefix)) return null | ||
| answers.push(lines[i].slice(prefix.length)) | ||
| } | ||
| return answers |
There was a problem hiding this comment.
Multiline answers break pairing
Medium Severity
Question answers are serialized with newlines between questions, but each answer is not escaped. A multiline “Something else” value adds extra lines, so parseQuestionAnswerMessage treats them as more questions, pairing fails, the user bubble stays visible, and the card never shows the answered recap after reload.
Reviewed by Cursor Bugbot for commit 040dea9. Configure here.
| const { copied, failed, failedCopyIds } = await executeChatFileBlobCopies(result.blobTasks, { | ||
| userId, | ||
| workspaceId: parent.workspaceId ?? undefined, | ||
| }) |
There was a problem hiding this comment.
Quota checked before copy completes
Low Severity
Forking checks storage quota from planned byte totals, then commits the chat and copies blobs afterward. Concurrent forks or other uploads can consume quota between the check and incrementStorageUsage, so copies can succeed even when the workspace is already at or over its limit.
Reviewed by Cursor Bugbot for commit 040dea9. Configure here.
Remove this branch's generated migrations so they can be regenerated against the latest staging schema with fresh numbering.
Resolve mothership executor, upload-test mocks, and regenerate tool schemas from the copilot staging catalog.
Remove the dev-generated migration so the invite branch can retain its manually hardened migration 260 and the remaining schema delta can be regenerated afterward.
# Conflicts: # apps/sim/lib/copilot/tools/handlers/materialize-file.ts # apps/sim/lib/mothership/inbox/executor.ts # apps/sim/lib/uploads/contexts/workspace/track-chat-upload.test.ts # apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts
Preserve the product invariant that Mothership chat files are not charged as workspace file storage after the billing storage merge.
Keep the manually hardened invite migration at 0260 and place the dev-only workspace_files.message_id change in a newly generated 0261.
4 participants
Summary
Brief description of what this PR does and why.
Fixes #(issue)
Type of Change
Testing
How has this been tested? What should reviewers focus on?
Checklist
Screenshots/Videos